Skip to content

fix(libc): create noreplace streams exclusively - #5949

Merged
adriadam10 merged 3 commits into
boykopovar:mainfrom
Ondra97:codex/fix-fiopen-exclusive-create
Oct 17, 2026
Merged

adriadam10 merged 3 commits into
boykopovar:mainfrom
Ondra97:codex/fix-fiopen-exclusive-create

Conversation

@Ondra97

@Ondra97 Ondra97 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What

C++ _Fiopen previously implemented noreplace by probing the existing file for reading before opening it for writing. A Windows file shared for writing but not reading makes that probe fail, so the later open can truncate the file. The separate probe also lets concurrent creators both succeed.

Keep Noreplace in the mode key for writing modes and select the existing exclusive fopen modes for its six valid combinations. Read-only modes continue to ignore Noreplace, as the existing guest_fiopen test requires. The combinations follow Microsoft STL's _Fiopen table; fopen already uses CREATE_NEW on Windows and exclusive host opening elsewhere. No separate existence probe remains.

Tested

Original validation at PR head d902afe2d on main e34c47921: Windows 11 Pro 10.0.26300, WinLibs GCC 15.2.0, Release. The write-shared truncation regression failed on main and passed with the fix. The test covered all six exclusive mode combinations, ate, EEXIST for an existing file, and twelve two-creator races with exactly one winner. The full default Windows build and four focused file tests passed.

Follow-up at head 3f95553e8, merged with main 5d9dcc42d:

Linux (VirtualBox VM), one guest_fiopen run Before (a70531c8 + new assertion) After (3f95553e8)
Read-only `In Binary Noreplace`
  • Linux (VirtualBox VM): Ubuntu 24.04.5 LTS, kernel 7.0.0-31-generic, GCC 13.3, Release, 8 vCPU / 8 GiB. Full BUILD_TESTING=ON build passed on the fixed source. ctest --test-dir build -R '^(guest_fiopen|file_streams|file_position|guest_posix_filesystem|guest_filesystem)$' --output-on-failure: 4/4 passed; guest_filesystem is not registered on Linux. Before the implementation fix, file_streams passed and guest_fiopen failed with the new assertion. Linux graphics/Vulkan tests: NOT RUN — intentionally excluded from VirtualBox VM validation. Native/bare-metal Linux and native Linux GPU were not tested.
  • Windows (native host): Windows 11 Pro 10.0.26300, AMD Ryzen 9 7900X, 31.7 GiB RAM, NVIDIA RTX 4070 Ti SUPER, WinLibs GCC 15.2.0, Release. ctest --test-dir build -R '^(guest_fiopen|file_streams|file_position|guest_filesystem|guest_posix_filesystem)$' --output-on-failure: 5/5 passed. The unmodified full build stops in a current-main AGC test because MinGW rejects alignas(65536) (object-file maximum 8192). A temporary, uncommitted adjustment to that unrelated test allowed the remaining targets to build; it was removed before this commit. Full CTest with that adjustment: 1283 passed, 3 failed, 14 skipped. agc_driver_multisample_fill_host_only passed on serial retry; guest_poll and guest_kqueue repeated their aborts. Their cause remains unverified.
  • Conventions and whitespace checks passed. No title or homebrew run. No performance claim.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw; no silent stubs added
  • No notes, investigation files or images added to the repository
  • New third-party code is a submodule built from source; none added
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes, Codex

Full platform runtime CI requires a write-access collaborator to dispatch it under repository policy.

@github-actions github-actions Bot added the libc label Oct 14, 2026
@Dextroid17

Copy link
Copy Markdown
Contributor

I ran this PR's test on Linux.

Setup: this PR's diff applied to main at 8cd937e (the PR's base is 421 commits behind it), default CMake build (CMAKE_BUILD_TYPE empty, unoptimized), clean environment. ctest -R '^file_streams$', 3 runs each. The test did not open a GPU device, so this is a CPU-only run.

  • with this PR: 3 of 3 pass.
  • with libc/src/FileAndHeap.cpp put back to main's version (the new test kept): 3 of 3 fail with the default "File stream check failed" message, so one of the shared checks fails (by reading, probably the EEXIST check or the two-creator race). The write-shared truncation check is Windows-only and did not run here.

So on Linux the new test catches the non-exclusive open, but not the Windows truncation case. I did not check Windows, macOS or a real GPU.

AI-assisted: yes

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 5668 core/libs/prx/libc/src/FileAndHeap.cpp
PR 6044 core/libs/tests/FileStreams.cpp
core/libs/tests/GuestFiopen.cpp
PR 6222 core/libs/tests/GuestFiopen.cpp

@nicolasfalesy

Copy link
Copy Markdown
Contributor

Tested on Linux (Arch), c++ (GCC) 16.2.1 20260810, Release, BUILD_TESTING=ON, main at 5d9dcc42d2ec5401aebd089028b8d4d7410a77b4.

This PR breaks the existing test guest_fiopen. On main it passes (2 of 2 full ctest -j8 runs). On main plus this PR it aborts (2 of 2):

Fiopen check failed at line 113

Line 113 of core/libs/tests/GuestFiopen.cpp requires that Open(file, In | Noreplace) returns a stream.

Cause: in FileAndHeap.cpp:346 the open flags now keep Noreplace for every mode. The modes table (FileAndHeap.cpp:343) only lists Noreplace together with Out, so In | Noreplace finds no entry and _Fiopen returns nullptr. A read-only open with Noreplace used to ignore the flag.

Suggested fix: keep Noreplace in open only when the mode writes, for example (mode & (In | Out | App | Trunc | Binary)) | (((mode & Noreplace) && (mode & (Out | App))) ? Noreplace : 0) | .... With that change guest_fiopen and file_streams pass on the merge.

AI-assisted: yes

@Ondra97

Ondra97 commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

@nicolasfalesy Thanks for the precise reproduction. I confirmed the read-only mode regression, kept Noreplace only for writing modes, and added a binary read-only regression case. On Linux (VirtualBox VM), the new assertion failed before the fix and passed after it; the full Release build and 4/4 focused file tests passed. Windows focused file tests passed 5/5. I pushed 3f95553e8 and updated the PR description with the full validation results and remaining test failures.

AI-assisted: yes. — Codex

@nicolasfalesy

Copy link
Copy Markdown
Contributor

Rechecked 3f95553e8 merged onto current main (bba8b293b), Linux (Arch, bare metal), c++ (GCC) 16.2.1 20260810, Release: guest_fiopen and file_streams both pass, 2 of 2 runs. The read-only Noreplace regression from my earlier comment is fixed here.

AI-assisted: yes

@adriadam10 adriadam10 self-assigned this Oct 16, 2026
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the _Fiopen mode table against Microsoft STL's fiopen.cpp, built and ran file_streams and guest_fiopen on Linux (both pass). file_streams fails without the change, and the read-only Noreplace case fixed in 3f95553 is covered in guest_fiopen. Confirmed on Linux by @Dextroid17 and @nicolasfalesy; merging.

@adriadam10

Copy link
Copy Markdown
Collaborator

Re-checked against current main (2fab88d): merges cleanly, file_streams and guest_fiopen pass on Linux, and the _Fiopen mode table matches Microsoft STL's fiopen.cpp. Confirmed on Linux by @Dextroid17 and @nicolasfalesy; merging.

@adriadam10
adriadam10 merged commit f3263df into boykopovar:main Oct 17, 2026
15 checks passed
@Ondra97
Ondra97 deleted the codex/fix-fiopen-exclusive-create branch October 17, 2026 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants